Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose the clock properties in CTFTrace #155

Merged
merged 1 commit into from
Sep 30, 2024
Merged

Expose the clock properties in CTFTrace #155

merged 1 commit into from
Sep 30, 2024

Conversation

siwei-zhang-work
Copy link
Contributor

@siwei-zhang-work siwei-zhang-work commented Sep 23, 2024 •
edited
Loading

There is the need to get clock scale of the trace.
This commit added the clock scale to the environment variables of the trace.

[Added] clock scale in getEnvironment() in CTFTrace
[Changed] moved clock offset into getEnvironment() in CTFTrace
[Deprecated] CtfTmfTrace.CLOCK_OFFSET, use CTFTrace.CLOCK_OFFSET instead.

Signed-off-by: Siwei Zhang [email protected]

@MatthewKhouzam
Copy link
Contributor

Hi Siwei, any thoughts on complementing getEnvironment with the frequency? It would require less code and no API changes.

@siwei-zhang-work siwei-zhang-work force-pushed the expose-clock branch 2 times, most recently from aa91f94 to e3c95bc Compare September 24, 2024 08:32
@siwei-zhang-work
Copy link
Contributor Author

siwei-zhang-work commented Sep 24, 2024 •
edited
Loading

Hi Siwei, any thoughts on complementing getEnvironment with the frequency? It would require less code and no API changes.

But the getEnvironment() refers to a specific env block in the metadata which doesn't include frequency. Also complementing getEnvironment() still requires exposing frequency from the CTFClock. I changed to adding the frequency in the getProperties(), and the frequency is exposed the same way as time offset. Does this seem cleaner? Otherwise I'll add the frequency in the getEnvironment().

@MatthewKhouzam
Copy link
Contributor

getEnvironment currently refers to env but it means all data that is not in events. I would recommend using it for all static information, also I will add get environment to TmfTrace. Trace Event should have this for metadata too. Maybe we can get a second opinion, but I would not add a new api for this instead of enhancing the output.

@siwei-zhang-work siwei-zhang-work force-pushed the expose-clock branch 2 times, most recently from 33532fd to bc108ad Compare September 27, 2024 10:36
@siwei-zhang-work siwei-zhang-work changed the title Expose the clock and frequency in CtfTmfTrace Expose the clock properties in CTFTrace Sep 27, 2024
@siwei-zhang-work
Copy link
Contributor Author

getEnvironment currently refers to env but it means all data that is not in events. I would recommend using it for all static information, also I will add get environment to TmfTrace. Trace Event should have this for metadata too. Maybe we can get a second opinion, but I would not add a new api for this instead of enhancing the output.

Sounds good, done.

MatthewKhouzam
MatthewKhouzam requested changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure you're not breaking the API. @deprecate instead.

Add clock scale to CTFTrace environment
a83d9cc 
There is the need to get clock scale of the trace.
This commit added the clock scale to the environment variables of the trace.

[Added] clock scale in getEnvironment() in CTFTrace
[Changed] moved clock offset into getEnvironment() in CTFTrace

Signed-off-by: Siwei Zhang <[email protected]>
bhufmann
bhufmann approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@MatthewKhouzam MatthewKhouzam merged commit 7039327 into eclipse-tracecompass:master Sep 30, 2024
4 checks passed
bhufmann added a commit to bhufmann/org.eclipse.tracecompass.incubator that referenced this pull request Oct 1, 2024
@bhufmann
server: Fix tests failing due to new trace properties added in TC
b588973 
The Trace Compass mainline PR below added a new trace properties to
which has to be accounted for in the REST server tests:

eclipse-tracecompass/org.eclipse.tracecompass#155

Signed-off-by: Bernd Hufmann <[email protected]>
@bhufmann bhufmann mentioned this pull request Oct 1, 2024
Merged
bhufmann added a commit to bhufmann/org.eclipse.tracecompass.incubator that referenced this pull request Oct 1, 2024
@bhufmann
server: Fix tests failing due to new trace properties added in TC
004164d 
The Trace Compass mainline PR below added a new trace properties to
which has to be accounted for in the REST server tests:

eclipse-tracecompass/org.eclipse.tracecompass#155

Signed-off-by: Bernd Hufmann <[email protected]>
@bhufmann bhufmann mentioned this pull request Oct 1, 2024
Merged
bhufmann added a commit to eclipse-tracecompass-incubator/org.eclipse.tracecompass.incubator that referenced this pull request Oct 1, 2024
@bhufmann
server: Fix tests failing due to new trace properties added in TC
149d2d7 
The Trace Compass mainline PR below added a new trace properties to
which has to be accounted for in the REST server tests:

eclipse-tracecompass/org.eclipse.tracecompass#155

Signed-off-by: Bernd Hufmann <[email protected]>
bhufmann added a commit to eclipse-tracecompass-incubator/org.eclipse.tracecompass.incubator that referenced this pull request Oct 1, 2024
@bhufmann
server: Fix tests failing due to new trace properties added in TC
d81578f 
The Trace Compass mainline PR below added a new trace properties to
which has to be accounted for in the REST server tests:

eclipse-tracecompass/org.eclipse.tracecompass#155

Signed-off-by: Bernd Hufmann <[email protected]>
adel-belkhiri pushed a commit to adel-belkhiri/org.eclipse.tracecompass.incubator that referenced this pull request Oct 15, 2024
@bhufmann @adel-belkhiri
server: Fix tests failing due to new trace properties added in TC
57e628e 
The Trace Compass mainline PR below added a new trace properties to
which has to be accounted for in the REST server tests:

eclipse-tracecompass/org.eclipse.tracecompass#155

Signed-off-by: Bernd Hufmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MatthewKhouzam MatthewKhouzam MatthewKhouzam approved these changes

@bhufmann bhufmann bhufmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@siwei-zhang-work @MatthewKhouzam @bhufmann